-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: basic Redis grammar, parseRedisQueryWithoutCursor function, tests #207
Conversation
}); | ||
|
||
test('should not report errors on multiple INCR commands', () => { | ||
const autocompleteResult = parseRedisQueryWithoutCursor('INCR test\nINCR 123'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We split queries by newline? Maybe it's better to force ;
in the end? This way it's gonna be easier for us to parse multiple commands too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't use semicolon, because it is valid to use in key/value name, e.g. SET key value;
will set value to value;
that's why Redis commands are separated by newline, that's also the way they do it in DataGrip
| command NEWLINE commands | ||
; | ||
|
||
command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we should call it command
, and not statement
? Statements
is a common name afaik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
const autocompleteResult = parseRedisQueryWithoutCursor('GET test\nGET 123'); | ||
|
||
expect(autocompleteResult.errors).toHaveLength(0); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to have a separete multiple commands test. Because in the end we want to test commands
rule 3fd0a09#diff-b96534631ee82ffc85910f193fcacb74b496e3324eef92e1d7121103f2321683R16
}); | ||
|
||
test('should not report errors on SET command with all argument', () => { | ||
const autocompleteResult = parseRedisQueryWithoutCursor('SET test test XX GET PXAT 123'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These XX
, EXAT
are declared in the redis grammar? Or is it our doing? How will we document this syntax in our documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's part of official syntax of course - https://redis.io/docs/latest/commands/set/
I don't see why we need to document Redis syntax in our own docs
; | ||
|
||
setCommand | ||
: SET keyName identifier (NX | XX)? GET? expirationClause? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is NX | XX
? Let's move it to a separate rule for improved reliability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job!
Closes #ISSUE_ID